Skip to content

Avoid memory corruption in arm context save/restore code #2276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
droe wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Avoid memory corruption in arm context save/restore code #2276

droe wants to merge 2 commits into from
+76 −10

Conversation

@droe
Copy link
Contributor

@droe droe commented Dec 20, 2025

Addresses multiple issues in the arm context save/restore code:

  • When restoring from context, do not clobber pointers in CPUARMState with data from the context buffer. This causes use-after-free bugs usually manifesting as double-free crashes.
  • When saving to context, do not copy pointers from CPUARMState to the context buffer, to avoid leaking pointers to the context. Instead, zero the respective area of the context buffer, to ensure that reg_read and reg_write can access fields outside of the copied range. Specifically, env->uc needs to be NULL when accessed for a context.
  • When restoring to a CPU with a different nr than the source CPU, copy min(nr, ctx_nr) instead of nothing at all.
  • Add regression tests checking all CPU models for arm and arm64 for trivial double-free crashes.

Fixes #2195

@droe droe force-pushed the droe/arm-context-fix branch from 3efb186 to 613df32 Compare December 20, 2025 23:13
wtdcode
wtdcode reviewed Jan 16, 2026
View reviewed changes
qemu/target/arm/unicorn_arm.c

#undef ARM_ENV_RESTORE
// Overwrite uc to our uc
env->uc = uc;
Copy link
Member

@wtdcode wtdcode Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this removed by design?

Copy link
Contributor Author

@droe droe Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We no longer clobber env->uc during restore, hence we do not need to fix env->uc to the correct uc after clobbering it with a potentially stale or mismatching pointer from the context.

@wtdcode
Copy link
Member

wtdcode commented Jan 16, 2026

LGTM except Specifically, env->uc needs to be NULL when accessed for a context.. Could you elaborate the reason?

@droe
Copy link
Contributor Author

droe commented Jan 16, 2026 •
edited
Loading

The reason is that AArch32 reg_write wants to write to env->uc->thumb when setting pc, and it checks if uc is NULL to check if it is operating on a live uc or a context, see:

unicorn/qemu/target/arm/unicorn_arm.c

Lines 440 to 443 in d4076e1

if (env->uc) {
// This can be NULL if env is a context
env->uc->thumb = (*(uint32_t *)value & 1);
}

reg_write writing to uc->thumb looks like a potential bug to me, but I did not want to investigate and touch that in this PR. Maybe this PR should address that too, what do you think?

@wtdcode
Copy link
Member

wtdcode commented Jan 16, 2026

The reason is that AArch32 reg_write wants to write to env->uc->thumb when setting pc, and it checks if uc is NULL to check if it is operating on a live uc or a context, see:

unicorn/qemu/target/arm/unicorn_arm.c

Lines 440 to 443 in d4076e1

if (env->uc) {
// This can be NULL if env is a context
env->uc->thumb = (*(uint32_t *)value & 1);
}

reg_write writing to uc->thumb looks like a potential bug to me, but I did not want to investigate and touch that in this PR. Maybe this PR should address that too, what do you think?

This points to a903fa1

Would you mind rebasing your branch to the latest dev so that we can have CI to run?

droe added 2 commits January 16, 2026 16:37
@droe
Avoid memory corruption in arm context save/restore code
f7606f7 
Addresses multiple issues in the arm context save/restore code:
-   When restoring from context, do not clobber pointers in CPUARMState
    with data from the context buffer.  This causes use-after-free bugs
    usually manifesting as double-free crashes.
-   When saving to context, do not copy pointers from CPUARMState to the
    context buffer, to avoid leaking pointers to the context.  Instead,
    zero the respective area of the context buffer, to ensure that
    reg_read and reg_write can access fields outside of the copied
    range.  Specifically, env->uc needs to be NULL when accessed for a
    context.
-   When restoring to a CPU with a different nr than the source CPU,
    copy min(nr, ctx_nr) instead of nothing at all.
-   Add regression tests checking all CPU models for arm and arm64 for
    trivial double-free crashes.

Fixes unicorn-engine#2195
@droe
Add cautionary comment for AArch64 uc->cpu_context_size
027015b
@droe droe force-pushed the droe/arm-context-fix branch from 2112f44 to 027015b Compare January 16, 2026 15:39
@droe
Copy link
Contributor Author

droe commented Jan 16, 2026

Rebased on latest dev.

@wtdcode
Copy link
Member

wtdcode commented Jan 16, 2026

mingw test failure is Github Action bug.

@droe
Copy link
Contributor Author

droe commented Jan 17, 2026

This points to a903fa1

I am not sure why thumb is a property of uc and not part of the CPU state, but thinking about it some more, the behaviour makes sense. I think this PR can go in as is (unless there are any concerns of course).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wtdcode wtdcode wtdcode left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@droe @wtdcode